Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sandboxed-process): support for esm files #945

Open
wants to merge 6 commits into
base: next
Choose a base branch
from

Conversation

roggervalf
Copy link
Collaborator

No description provided.

Copy link
Contributor

@manast manast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vasinl124
Copy link

I'm sorry, could you please tell me when you're going to merge this?

@grahamb
Copy link

grahamb commented Jul 5, 2022

Are there plans to pull this in any time soon? As far as I can see it works fine. Not having ESM support for sandboxed workers is a huge PITA.

grahamb added a commit to grahamb/bullmq that referenced this pull request Jul 5, 2022
@manast
Copy link
Contributor

manast commented Jul 7, 2022

@grahamb I wish it was so easy as to just add support to ESM, the problem is that it breaks other stuff... I wrote this piece of post maybe it is useful to you: https://blog.taskforce.sh/using-typescript-with-bullmq/

@grahamb
Copy link

grahamb commented Jul 9, 2022

I saw that post, and no, unfortunately it doesn't help. My workaround for now was to alter the compiled CJS build to use await import rather than require, and host a tarball of that myself; this works exceptionally well, but not optimal as now I have to maintain my own fork and redo a build. I guess I need to search for a replacement jobs processor if BullMQ isn't going to be compatible with modern node 😕

@manast
Copy link
Contributor

manast commented Jul 10, 2022

I guess I need to search for a replacement jobs processor if BullMQ isn't going to be compatible with modern node

Or you can provide a working solution too.

@patrickheeney
Copy link

I wish it was so easy as to just add support to ESM, the problem is that it breaks other stuff...

@manast I've been poking around all the issues and I see when ESM is added it gets reverted due to various reasons. Is there a high level explanation as to what is breaking when require changes to await import for sandboxed processors? It is hard to tell from the PRs and issues since some users report it working.

@manast
Copy link
Contributor

manast commented Aug 27, 2022

This is due the way Node works. Depending on the context where you put Bull (i.e. the settings in the package.json of the application depending on Bull) it may break if we enable ESM. It is super hard to make it compatible with all current setups supported by Node.

@patrickheeney
Copy link

@manast What about a separate build? Or should the existing esm build support multiple setups? I am trying to understand what blockers exist in this library or node itself order to support this feature?

@manast
Copy link
Contributor

manast commented Aug 27, 2022

I am trying to understand what blockers exist in this library or node itself order to support this feature?

Me too. It is super difficult to get it right, I spent plenty of hours to no avail, besides it is completely ilogical there does not seem to exist any proper way to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants